@juherr i will take a look at this ASAP when i am finished with some other duties.

on a more general note: i had a conversation with @florinmandache after his initial big PR. he said that he has many things going on and is short on time, and he wanted to contribute on good will. since he is a busy person, he was not interested in follow-up, review rounds and eventual clean-ups (i.e. the usual process of PR and reviewing). due to the substantial nature of the contribution, i accepted it as-is and took it upon me to do whatever is necessary to bring these features to main branch.

in this context, i thank you for doing one part of it.

@goekay Thanks for the clarification. I think Gemini did an interesting initial job exploring the different topics, but it definitely needs more work based on what I’ve seen so far (especially regarding OCPP 1.6 Security and OICP). Since OCPI and OCPP 2.x are an even tougher challenge, and my time is just as limited, I’ll let you take the first shot on those ones 😉

FYI @juherr: steve-community/ocpp-jaxb#18

naming of the json schema files differ a bit from the original (see links from comment). do you know why?

Ah, you mean the request suffix in some filenames?
I personally find it cleaner and more robust to change it, but your approach seems to work fine too 👍

i did some "low-hanging fruit" refactoring to reduce the size of the PR (i.e. moving the data models and their generation to ocpp-jaxb library). will do some more. i think this is very important in order to able to properly review the PR and separate the good parts from inaccurate AI slop. while being on this...

i did a litmus test and tried to understand how "http basic auth" (security profile 1) is supposed to work in the impl. ChargePointRepository has 2 new methods isRegistered(String chargeBoxId) and validatePassword(String chargeBoxId, String password), but no one is using them.

moreover, there is no change in SecurityConfiguration or OcppWebSocketHandshakeHandler (more emphasis on this), such that the handler retrieves the basic auth header from request and validates the password against the DB.

WIP - Implementation status of new operations

ignoring and removing the following method completely, since it has no correlation with reality IMO:

    private static String determineSeverity(String eventType) {
        if (eventType == null) {
            return "INFO";
        }

        var upperType = eventType.toUpperCase();

        if (upperType.contains("ATTACK") || upperType.contains("BREACH") || upperType.contains("TAMPER")) {
            return "CRITICAL";
        }

        if (upperType.contains("FAIL") || upperType.contains("ERROR") || upperType.contains("INVALID")
            || upperType.contains("UNAUTHORIZED") || upperType.contains("REJECT")) {
            return "HIGH";
        }

        if (upperType.contains("WARNING") || upperType.contains("EXPIR")) {
            return "MEDIUM";
        }

        return "INFO";
    }

the eventType references type field in SecurityEventNotification message with the following value possibilities:

the table actually continues in the next page with more types, but there is no type with INFO, ATTACK (heh?), BREACH (what?) etc. so this whole thing is made up.

moreover, the PR invents new types (that are not in the list), such as SignCertificateError with a HIGH severity, for ex in

            securityRepository.insertSecurityEvent(
                chargeBoxIdentity,
                "SignCertificateError",
                DateTime.now(),
                "Error signing CSR: " + e.getMessage(),
                "HIGH"
            );

these usages feel also wrong. actually, the motivation is nice: if there is an error during the signing of certificate, create a security event and store it in DB. but, these security events are NOT coming from station, and intermixing these with regular events is problematic IMO.

@juherr do you happen to know whether these are retrofits coming from ocpp 2.x ?

@juherr do you happen to know whether these are retrofits coming from ocpp 2.x ?

I haven’t looked deeply into OCPP 2.x or even the current PR once I understood where it originated from.
My assumption is that Gemini applied a pattern from another context by mistake.

According to the whitepaper, only critical events are supposed to be sent by the station — and I believe all of those should indeed be stored.
Happy to help if you can elaborate a bit more on your question.

so, i can assume that these additions were a mistake and i can remove them

• additional severity column derived from security event type
• additional security event inserts (triggered by backend/steve) when local certificate-related operation fails

?

In my opinion, security events triggered by the backend are a separate concern, and a warning log should be enough for now.
At the very least, an internal event could be raised so that any fork can decide how it wants to handle it.